xds: add internal ResourceNameFunc server option - #9288
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9288 +/- ##
==========================================
+ Coverage 83.00% 83.08% +0.08%
==========================================
Files 422 424 +2
Lines 35085 35252 +167
==========================================
+ Hits 29121 29289 +168
- Misses 4441 4448 +7
+ Partials 1523 1515 -8
🚀 New features to boost your workflow:
|
726a5b4 to
5c0a214
Compare
easwars
added
Area: xDS
eshitachandwani
added
Status: Requires Reporter Clarification
and removed
Status: Requires Reporter Clarification
labels
ran-su
changed the title
eshitachandwani
assigned
eshitachandwani and
ran-su and unassigned
ran-su and
eshitachandwani
eshitachandwani
left a comment
eshitachandwani
left a comment
There was a problem hiding this comment.
LGTM modulo 2 comments. Adding @easwars for a second review.
| if req.GetTypeUrl() == version.V3ListenerURL && | ||
| cmp.Equal(req.GetResourceNames(), []string{wantResourceName}) { |
There was a problem hiding this comment.
Nit: Please don't break up lines this way. Go does not impose line length restriction for code lines. See: https://google.github.io/styleguide/go/guide#line-length
| srv, err := NewGRPCServer( | ||
| internalserver.OverrideListenerResourceName(func(addr net.Addr) string { | ||
| addrCh <- addr | ||
| return wantResourceName | ||
| }), | ||
| BootstrapContentsForTesting(bs), | ||
| ) |
There was a problem hiding this comment.
Nit: Same here. Please don't break this up like this. Pull the server option out into a local variable so that the call to NewGRPCServer can be comfortably placed on a single line. Thanks.
| srv.Stop() | ||
| stopped = true | ||
| // Verify that Serve returns after the server is stopped. | ||
| select { | ||
| case err := <-serveErrCh: | ||
| if err != nil { | ||
| t.Fatalf("Serve() returned error: %v", err) | ||
| } | ||
| case <-ctx.Done(): | ||
| t.Fatal("Timeout waiting for Serve() to return") | ||
| } |
There was a problem hiding this comment.
This is not pertinent to what is being tested here, right? This should be covered by other tests that focus on Serve returning once Stop is called. So, I would remove all this logic from here and defer a call to srv.Stop and forget about it.
| newGRPCServer = func(...grpc.ServerOption) grpcServer { return fs } | ||
| defer func() { newGRPCServer = origNewGRPCServer }() | ||
|
|
||
| addrCh := make(chan net.Addr, 1) |
There was a problem hiding this comment.
Nit: Rename this as lisAddrCh since this variable is used long after it is defined and therefore it is better to have a more descriptive name.
easwars
left a comment
easwars
left a comment
There was a problem hiding this comment.
LGTM, modulo one minor nit in the test.
| // template is absent, and that its returned name is used for the LDS watch. | ||
| func (s) TestServer_OverrideListenerResourceNameOverridesMissingTemplate(t *testing.T) { | ||
| const wantResourceName = "xdstp://foo/bar" | ||
| ldsResourceNameReceived := grpcsync.NewEvent() |
There was a problem hiding this comment.
Nit: Instead of this being a grpcsync.Event, this could be a chan []string. Then, the OnStreamRequest implementation does not need to validate the resource name. It will simply push the received slice of resource names onto this channel. And the main test goroutine at the end will read the value out of the channel and compare it.
The advantage with the approach that I'm suggesting is that if the resource names does not match with the expected value, then the test error message can include it and that will help with debugging when the test fails.
Also, please be aware that this method will be invoked multiple times during the test:
- The first time the server requests for the LDS resource
- The ACK for the above resource
- When the server is shutting down, there is a race where the xDS client might end up sending an LDS request with an empty set of resource names.
So, the write to the channel needs to be guarded with a test context:
select {
case ldsResourceNames <- req.GetResourceName():
case <-ctx.Done()
}Where ctx is the overall test context which is currently defined as
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()and this would need to be moved to the top.
|
@eshitachandwani I have addressed the comments. Could you please take another look at this PR? If this version looks good to you, could you please help me merge it? Thanks. |
|
Hey @ran-su could you please rebase your PR on master to get the codecov job passing. It has been fixed. |
Introduces `ResourceNameFunc` callback option to `NewGRPCServer` for overriding server listener resource name generation. Exposes `UnderlyingServer()` method to retrieve the inner target server for chaining.
5a68c82 to
12f67c6
Compare
3 participants
Adds an internal
ResourceNameFuncserver option that allows override the LDS listener resource name.RELEASE NOTES: none